-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fits event lists #2
Conversation
196405f
to
d6c8782
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, I added just some comments :) I hope this is somehow useful
-------- | ||
>>> gtis = [[0, 30], [50, 60], [80, 90]] | ||
>>> new_gtis = split_gtis_at_indices(gtis, 1) | ||
>>> assert np.allclose(new_gtis[0], [[0, 30]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to change with a clearer line showing the result/return in the example. assert np.allclose(... is kind of tricky to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little tricky at the moment after Numpy 2.0. They changed the repr
of numpy arrays, so that, e.g. when you print out the value of the array in the doctest it appends np.float64
stuff, and if you had just put some [[0, 30]]
as the expected output, the test would fail. If you take it one step further and you just say, e.g. >>> np.allclose(new_gtis[0], [[0, 30]])
it will not say True
, but np.True_
😠
See, e.g.: astropy/astropy#15095
Astropy went for showing the updated format, but this means we cannot test, e.g., in Numpy 1.26.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see, then let's leave it like this.
stingray/gti.py
Outdated
|
||
exposure_edges = np.asarray(exposure_edges) | ||
|
||
total_exposure = last_exposure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave the starting total_exposure as different for the last_exposure. It could be useful for the use to check their work maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They serve different purposes: the initial total_exposure
was an approximate measurement. I hope the new naming clarifies this difference
d064b2d
to
f762d35
Compare
f4ae6a1
to
af72f8a
Compare
f8fe13b
to
888aee1
Compare
…achetti-patch-1 Add pyOpenSci review badge
Accept file names instead of hdulist Allow for shorter exposure cuts than GTIs
Additional Inference of format for fits formats
888aee1
to
8fb4213
Compare
…irects Fix redirects in docs; make better warning about Numba
…to other kinds of timeseries
…setuptools Remove setuptools from runtime dependencies
…nto fits_event_lists
Merged upstream! StingraySoftware#834 |
Introduces a new FITS reader class, that lazy loads the data until a slice is required (in which case, the wanted StingrayTimeseries object is created).